What the bug is

cleanup_and_save_logs is registered as a generic EXIT trap at line 101 (trap cleanup_and_save_logs EXIT), so it runs on every script exit — both success (exit 0) and failure. The new annotation block (lines 76–94) is guarded by only two conditions:

1. [[ -s "$err_file" ]] — stderr file is non-empty (line 66 enclosing block)
2. [[ -n "${GITHUB_ACTIONS:-}" ]] — running under Actions (line 76)

Neither indicates that the job actually failed. There is no $? capture and no exit-code gate.

Why a successful run will hit it

SGLang/vLLM multinode jobs with MoRI + NCCL + SLURM essentially always produce non-empty stderr on success:

• tqdm writes progress bars to stderr by default
• NCCL/RCCL prints INFO banners to stderr
• warnings.warn(...) writes to stderr
• SGLang/vLLM server INFO logs often route to stderr
• srun --unbuffered and docker stop $(docker ps -a -q) || true (job.slurm) leak banners and "no such container" messages to stderr — the trailing || true swallows the exit code but not the output
• The submit.sh wrapper sets --error=slurm_job-%j.err, so all of the above lands in the file the trap inspects

Two false-positive paths on green runs

1. Pattern match (line 84): grep -qiE "MoRI|mori_conn|pd[- ]?disagg" matches case-insensitively against the literal string MoRI anywhere in stderr. For a MoRI-based benchmark this is effectively guaranteed (framework name appears in INFO/log lines and startup banners). Result: ::error title=AMD disagg job <id> failed::transport-flake: MoRI KV-transport error.
2. Else branch (line 92): Even when no class matches, any non-empty unmatched stderr emits ::error title=AMD disagg job <id> failed::Unclassified failure - see last 100 lines of slurm .err above.

Either path renders a red banner in the Actions UI claiming the job FAILED on a run whose exit code is 0 and whose result JSON was written normally.

Step-by-step proof

1. Script runs, trap cleanup_and_save_logs EXIT is set (line 101).
2. Multinode benchmark completes successfully, exit code 0.
3. cleanup_and_save_logs fires on EXIT.
4. err_file=$BENCHMARK_LOGS_DIR/slurm_job-${JOB_ID}.err is non-empty (contains NCCL/MoRI INFO banners + tqdm + srun stderr).
5. [[ -s "$err_file" ]] → true → enter the block.
6. [[ -n "${GITHUB_ACTIONS:-}" ]] → true (we are in CI) → enter annotation block.
7. First three patterns (Model not found, ReadTimeout, Fp8BlockwiseQuant) don't match a clean run.
8. Fourth elif: grep -qiE "MoRI|mori_conn|pd[- ]?disagg" "$err_file" → matches the literal "MoRI" in any startup log line → sig="transport-flake: MoRI KV-transport error".
9. Line 90 emits ::error title=AMD disagg job <id> failed::transport-flake: MoRI KV-transport error (see slurm .err artifact).
10. Actions UI shows a red error banner on a successful, exit-0 run.

Existing code already acknowledges this

The pre-existing if [[ -s "$err_file" ]] guard at line 66 plus the unconditional tail -100 block already implicitly acknowledges that .err is routinely non-empty on non-failure runs (otherwise the tail print wouldn't be conditional). What the new block does is escalate that benign non-emptiness into a misleading ::error:: annotation.

Impact

The annotation directly defeats the PR's stated goal ("surface real failure class", "fail loudly and legibly"). After a few green-but-red runs, reviewers learn to ignore the annotation entirely, and the next real failure looks identical to the prior false positives.

Fix

Capture the trap-entry exit code as the very first line of cleanup_and_save_logs and gate only the ::error:: emission on it. The diagnostic .err tail (lines 67–69) is fine to leave unconditional since it's only printed output, not an Actions annotation:

cleanup_and_save_logs() {
    local rc=$?  # MUST be first statement
    # ... existing cp/tail logic unchanged ...
    if [[ $rc -ne 0 && -n "${GITHUB_ACTIONS:-}" && -s "$err_file" ]]; then
        # existing classifier + ::error:: emission
    fi
}